Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clarify the conditions to expose sensor readings #347

Merged
merged 1 commit into from
Mar 1, 2018
Merged

Clarify the conditions to expose sensor readings #347

merged 1 commit into from
Mar 1, 2018

Conversation

pozdnyakov
Copy link

@pozdnyakov pozdnyakov commented Feb 28, 2018 •
edited by pr-preview bot
Loading

This patch consolidates all the necessary conditions to
expose sensor readings to a single list at
https://w3c.github.io/sensors/#can-expose-sensor-readings
in order to make them more explicit and improve readability.

It also brings clarifications to the Sensor model description
and minor editorial changes (corrected references to the definitions
from the PERMISSIONS specification).

Preview | Diff

alexshalamov
alexshalamov approved these changes Feb 28, 2018
View reviewed changes
Copy link

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with nit

index.bs
- The given document is a [=responsible document=] of a [=secure context=].
- For each [=permission name=] from the [=sensor type=]'s associated
[=sensor permission names=] [=ordered set|set=], the corresponding permission's
[=permission state|state=] is "granted".
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[="granted"=]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we represent enum values like this through the whole document, so I would keep this consistency so far.

anssiko
anssiko approved these changes Feb 28, 2018
View reviewed changes
Copy link
Member

@anssiko anssiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a grammar nit and a comment re required changes to concrete specs.

index.bs Outdated
@@ -771,9 +781,9 @@ A [=sensor type=] has a [=ordered set|set=] of <dfn export>associated sensors</d
A [=sensor type=] may have a [=default sensor=].

A [=sensor type=] has a [=set/is empty|nonempty=] [=ordered set|set=] of associated
{{PermissionName|PermissionNames}} referred as <dfn export>sensor permission names</dfn>.
[=permission names=] referred as <dfn export>sensor permission names</dfn>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

referred to as

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

index.bs
@@ -1637,8 +1645,8 @@ each [=sensor type=] in [=extension specifications=]:
Its [=attributes=] which expose [=sensor readings=] are [=read only=] and
their getters must return the result of invoking [=get value from latest reading=]
with <strong>this</strong> and [=attribute=] [=identifier=] as arguments.
- A {{PermissionName}}, if the [=sensor type=] is not representing
[=sensor fusion=] (otherwise, {{PermissionName|PermissionNames}}
- A [=permission name=], if the [=sensor type=] is not representing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will you submit PRs to concrete sensor specs accordingly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I'll do it.

Clarify the conditions to expose sensor readings
e955ea0 
This patch consolidates all the necessary conditions to
expose sensor readings to a single list at
https://w3c.github.io/sensors/#can-expose-sensor-readings
in order to make them more explicit and improve readability.

It also brings clarifications to the Sensor model description
and minor editorial changes (corrected references to the definitions
from the PERMISSIONS specification).
@pozdnyakov pozdnyakov force-pushed the clarify_expose_readings_conditions branch from 05684a2 to e955ea0 Compare March 1, 2018 08:03
@pozdnyakov pozdnyakov merged commit 9386d2e into w3c:master Mar 1, 2018
@anssiko anssiko mentioned this pull request Mar 2, 2018
Closed
23 tasks
rakuco pushed a commit that referenced this pull request Feb 16, 2024
Remove note about sharing data between platform sensors
8839c23 
The original note was added in #267 and expanded in #347, but its advice is
impractical:
- Sharing the activated sensors objects between multiple browsing
  contexts/documents/windows means these Sensor objects could potentially be
  shared by contexts in different top-level traversables (i.e. different
  tabs).
  Furthermore, if "can expose sensor readings" passes for one context but
  not the other, "update sensor reading" would still invoke "report latest
  reading updated" with sensors that cannot expose sensor readings.
- Similarly, if the latest reading map is shared between multiple contexts,
  an update would affect all contexts, including those for which "update
  sensor reading" should not have been invoked in the first place (e.g. two
  pages with the same origin share the latest readings map, but only one is
  visible; updates to the latest reading map would be accessible from the
  other as well).

PR #267 also made the "platform sensor" concept used in this section
per-browsing context (although in a very confusing way), which on its own is
a stricter requirement than what the note allowed, so we can drop the note
without making things less secure.

Incidentally, this also gets rid of one of the usages of "browsing context"
in the spec, which helps with #444.
@rakuco rakuco mentioned this pull request Feb 16, 2024
Merged
rakuco pushed a commit that referenced this pull request Feb 17, 2024
Remove note about sharing data between platform sensors (#482)
ad110bd 
The original note was added in #267 and expanded in #347, but its advice is
impractical:
- Sharing the activated sensors objects between multiple browsing
  contexts/documents/windows means these Sensor objects could potentially be
  shared by contexts in different top-level traversables (i.e. different
  tabs).
  Furthermore, if "can expose sensor readings" passes for one context but
  not the other, "update sensor reading" would still invoke "report latest
  reading updated" with sensors that cannot expose sensor readings.
- Similarly, if the latest reading map is shared between multiple contexts,
  an update would affect all contexts, including those for which "update
  sensor reading" should not have been invoked in the first place (e.g. two
  pages with the same origin share the latest readings map, but only one is
  visible; updates to the latest reading map would be accessible from the
  other as well).

PR #267 also made the "platform sensor" concept used in this section
per-browsing context (although in a very confusing way), which on its own is
a stricter requirement than what the note allowed, so we can drop the note
without making things less secure.

Incidentally, this also gets rid of one of the usages of "browsing context"
in the spec, which helps with #444.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexshalamov alexshalamov alexshalamov approved these changes

@anssiko anssiko anssiko approved these changes

@rwaldron rwaldron Awaiting requested review from rwaldron

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pozdnyakov @anssiko @alexshalamov